Skip to content

doc: Provide bad/better examples for access() and exists()#7832

Closed
dfabulich wants to merge 1 commit into
nodejs:masterfrom
dfabulich:exists-access-examples
Closed

doc: Provide bad/better examples for access() and exists()#7832
dfabulich wants to merge 1 commit into
nodejs:masterfrom
dfabulich:exists-access-examples

Conversation

@dfabulich

@dfabulich dfabulich commented Jul 22, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jul 22, 2016
Comment thread doc/api/fs.md Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As opposed to saying should not be used, perhaps something a bit softer...

Using `fs.acess()` to check for the existence of a file before calling `fs.open()`,
`fs.readFile()`, or `fs.writeFile()` is not recommended because doing so...

@jasnell

jasnell commented Aug 5, 2016

Copy link
Copy Markdown
Member

Thank you for this, left some comments! Sorry it has take so long for someone to take a look and get back to you.

@dfabulich dfabulich force-pushed the exists-access-examples branch from c3c11b3 to ccec465 Compare August 9, 2016 18:51
@dfabulich

Copy link
Copy Markdown
Contributor Author

@jasnell Thanks for your feedback. I've incorporated your comments in the latest draft.

@jasnell

jasnell commented Aug 11, 2016

Copy link
Copy Markdown
Member

@dfabulich .. thank you! will be able to go through in detail again tomorrow :-)

@Fishrock123

Copy link
Copy Markdown
Contributor

ping @jasnell

@jasnell

jasnell commented Aug 22, 2016

Copy link
Copy Markdown
Member

Whoops, let this one slip off my radar. LGTM

@jasnell

jasnell commented Aug 24, 2016

Copy link
Copy Markdown
Member

ping @nodejs/documentation

Comment thread doc/api/fs.md Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe split the code blocks and put these pseudo-headings in bold just above them?

@addaleax

Copy link
Copy Markdown
Member

LGTM with a suggestion, thanks for putting these together!

Comment thread doc/api/fs.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the else, we can unindent the next function call.

@dfabulich dfabulich force-pushed the exists-access-examples branch from ccec465 to 80d6ec3 Compare August 31, 2016 23:50
@dfabulich

Copy link
Copy Markdown
Contributor Author

Incorporated feedback from @addaleax and @thefourtheye

@jasnell

jasnell commented Sep 1, 2016

Copy link
Copy Markdown
Member

LGTM! will get this landed!

jasnell pushed a commit that referenced this pull request Sep 1, 2016
PR-URL: #7832
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell

jasnell commented Sep 1, 2016

Copy link
Copy Markdown
Member

Landed in 143d38c. Thank you!

@jasnell jasnell closed this Sep 1, 2016
@Fishrock123 Fishrock123 mentioned this pull request Sep 6, 2016
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
PR-URL: nodejs#7832
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
PR-URL: #7832
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
PR-URL: #7832
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants